Show pinned articles in the articles list if 4 are already selected #942
Conversation
|
Thanks @ash-jc-allen this is a really great catch. However, I wonder if this is the correct approach as the pinned articles will only ever be the first pulled from the database with the Perhaps we should, instead, only allow n articles to be pinned at any one time? What do you think @ash-jc-allen and @driesvints? |
|
That's a great point! I did consider using that type of approach but wasn't too sure what you guys would think. If you think that'd be a better approach, I'm more than happy to close this PR and then make the changes in a separate branch 🙂 |
|
Let's see what @driesvints thinks when he gets a chance to check in |
|
Hey all. Thanks @ash-jc-allen, this is a great catch. I do agree with @joedixon though to limit pinned articles to max 4. Can we make that change instead? |
|
Hey @driesvints, that's no problem at all! I've had a crack at making these changes in #943 🙂 |
3 participants
Hey!
This PR proposes a small change that will include pinned articles in the regular articles list if there are more than 4 pinned articles.
For a bit of context, I have 19 articles published on laravel.io. However, you can only actually see 18 (or at least that's all I can see haha). One of them is in the pined list at the top. The other 17 are in the regular articles list. From what I can gather, I think it's because this article is currently pinned: https://laravel.io/articles/how-to-add-breadcrumbs-to-your-laravel-website
So from my guess, I think this article will be in a sort of "limbo" state. I don't think it'll get picked up by the pinned articles query because there are newer pinned articles. I also don't think it'll get picked up by the regular articles query because the article is
is_pinned.With my new change, it proposes that if a pinned article isn't included in the 4 pinned articles, it'll then be included int the regular article listing.
I'm hoping what I've done is right (and actually needed/wanted haha!). If this is something you do think might be worth merging, I'd be more than happy to make any changes if needed 😄
I've added some tests to make sure it works programmatically. But I've not done any manual testing because my local env is a little bit broken at the moment haha! So I do apologise if I've done anything wrong or broken anything!